Conversation
|
Sidebar: would be nice to get the CI status inline here. I think that means bumping some API keys for the integration? Been a while since I've set it up. Nevertheless, tests are green here: https://circleci.com/gh/notjosh/sirius/5 |
joerick
left a comment
joerick
left a comment
There was a problem hiding this comment.
Nice one. I'm interested to see how this turns out.
Just one thing needs fixing here, I think.
BTW, I added PR test runs to Circle, was just a missing checkbox on the settings.
sirius/coding/templating.py
Outdated
| # https://pypi.python.org/pypi/scrubber | ||
|
|
||
| def default_template(raw_html, from_name): | ||
| def default_template(raw_html, from_name, date=datetime.datetime.now()): |
There was a problem hiding this comment.
datetime.datetime.now() will be evaluated only once, when the function is defined. Better to assign None as a default and test if date is None: in the function body to set a default
There was a problem hiding this comment.
Huh, I didn't know that! Good catch, and noted for future. Fixed in 044be3b
🎉 |
joerick
left a comment
joerick
left a comment
There was a problem hiding this comment.
Sorry for the delay, been busy! Looks good, merge whenever you're ready :)
|
I could use faster tests, so I'm gonna merge :) |
2 participants
The comments on #8 made me think more about image snapshot tests, to make sure we're not accidentally causing regressions in the HTML->image chain(s). Since #8 changes the renderer, it's probably a good idea to get these tests in sooner rather than later.
There are only a couple of simple tests for now, but we can add cases when we find issues, and it's enough (imo!) to catch any obvious issues.
Couple of things:
make ci-snapshot-updatetask to bump the snapshots